Refactor command line argument parsing with gflags by jovany-wang · Pull Request #4676 · ray-project/ray

jovany-wang · 2019-04-20T10:57:21Z

What do these changes do?

Integrate gflags and refine the command line arguments parsing. This can make us more easily to add a option to raylet.

Related Issue

This can close #2280.

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-04-22T12:14:53Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/549/
Test PASSed.

AmplabJenkins · 2019-04-22T12:28:58Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/550/
Test PASSed.

AmplabJenkins · 2019-04-22T18:10:20Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13908/
Test FAILed.

robertnishihara · 2019-04-22T21:56:08Z

Nice, this is much cleaner. Shall we use gflags? Some discussion in #2280.

jovany-wang · 2019-04-23T03:33:04Z

@robertnishihara The purpose of this PR is to implement this with minimizing dependencies.
If you don't think gflag is heavy, I'd also like to using it.

Refine Enable in worker runner. Fix Fix sh scripts/format.sh Add doc comment. Fix lint

AmplabJenkins · 2019-04-23T06:57:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/557/
Test PASSed.

AmplabJenkins · 2019-04-23T08:14:12Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/558/
Test PASSed.

AmplabJenkins · 2019-04-23T08:28:27Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/559/
Test PASSed.

AmplabJenkins · 2019-04-23T09:46:59Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/561/
Test PASSed.

AmplabJenkins · 2019-04-23T10:26:56Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13923/
Test FAILed.

AmplabJenkins · 2019-04-23T11:00:09Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13924/
Test FAILed.

raulchen · 2019-04-23T11:03:50Z

python/ray/services.py

+        "-static_resource_list={}".format(resource_argument),
+        "-config_list={}".format(config_str),
+        "-python_worker_command={}".format(start_worker_command),
+        "-java_worker_command={}".format(java_worker_command),


worker command contains spaces, will this be an issue?

note that we can pass "-java_worker_command" and java_worker_command as 2 separate args, then we don't need to worry about spaces.

It works fine. The value of java_worker_command can be passed into raylet smoothly in this way.(The reason is similar to the 2 separated args you mentioned.)

raulchen · 2019-04-23T11:06:00Z

java/runtime/src/main/java/org/ray/runtime/runner/RunManager.java

-        buildPythonWorkerCommand(), // python worker command
-        buildWorkerCommandRaylet(), // java worker command
-        redisPasswordOption
+        String.format("-raylet_socket_name=%s", rayConfig.rayletSocketName),


I think the convention is to use double dashes for full names (--raylet_socket_name). why does gflag use single dash? Does gflag also support double?

gflags supports double dashes too.

raulchen · 2019-04-23T11:08:28Z

src/ray/raylet/main.cc

+DEFINE_bool(disable_stats, false, "Whether disable the stats.");
+DEFINE_string(stat_address, "127.0.0.1:8888", "The address that we report metrics to.");
+DEFINE_bool(disable_stdout_exporter, true,
+            "Whether disable the stdout exporter for stats.");


maybe change this to enable_stdout_exporter with default value false. Then we can just use --enable_std_exporter, instead of --disable_stdout_exporter=false.

AmplabJenkins · 2019-04-23T12:21:55Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13926/
Test FAILed.

AmplabJenkins · 2019-04-23T13:46:19Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/566/
Test PASSed.

AmplabJenkins · 2019-04-23T13:53:35Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/567/
Test PASSed.

AmplabJenkins · 2019-04-23T16:00:39Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13932/
Test FAILed.

AmplabJenkins · 2019-04-23T16:01:16Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13931/
Test FAILed.

robertnishihara · 2019-04-23T16:32:12Z

Using gflags seems ok to me. Is it particularly heavyweight? If it turns out to be an issue, then we can switch.

pcmoritz · 2019-04-23T17:42:14Z

I think gflags is good to use, it is already linked in because of gtest as far as I know.

jovany-wang · 2019-04-24T02:30:29Z

Thanks @pcmoritz and @robertnishihara . I have already switched to gflags.

raulchen

Lint check failed

AmplabJenkins · 2019-04-24T03:03:14Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/575/
Test PASSed.

AmplabJenkins · 2019-04-24T04:04:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/576/
Test PASSed.

robertnishihara · 2019-04-24T04:13:55Z

BUILD.bazel

        exclude = [
            "src/ray/util/logging_test.cc",
            "src/ray/util/signal_test.cc",
+            "src/ray/util/util_test.cc",


This no longer exists, right?

Nice catch!

AmplabJenkins · 2019-04-24T04:31:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/577/
Test PASSed.

AmplabJenkins · 2019-04-24T05:32:30Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13941/
Test FAILed.

AmplabJenkins · 2019-04-24T06:36:02Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13943/
Test FAILed.

AmplabJenkins · 2019-04-24T06:40:56Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13942/
Test FAILed.

jovany-wang force-pushed the command_line_args_parser branch 2 times, most recently from 6f186f9 to 1a82e84 Compare April 22, 2019 12:12

jovany-wang force-pushed the command_line_args_parser branch from 6cc00e6 to c7de75d Compare April 23, 2019 03:53

Add CommandLineArgs class.

ccf6728

Refine Enable in worker runner. Fix Fix sh scripts/format.sh Add doc comment. Fix lint

jovany-wang force-pushed the command_line_args_parser branch from c7de75d to ccf6728 Compare April 23, 2019 03:54

Use gflag

8276ce1

jovany-wang added 2 commits April 23, 2019 15:33

Fix

a7e98a5

Update commands.

d0edc94

add gflags to raylet_monitor

069eabf

jovany-wang changed the title ~~[WIP] Refactor command line argument parsing of raylet~~ Refactor command line argument parsing with gflags Apr 23, 2019

ray-project deleted a comment from AmplabJenkins Apr 23, 2019

jovany-wang requested a review from robertnishihara April 23, 2019 08:34

Fix lint

f094cc2

raulchen reviewed Apr 23, 2019

View reviewed changes

Use double dashes.

3b53d1d

address comments.

bd0955a

raulchen reviewed Apr 24, 2019

View reviewed changes

Fix linting

40c9b1d

raulchen approved these changes Apr 24, 2019

View reviewed changes

Merge branch 'master' into command_line_args_parser

3202391

robertnishihara reviewed Apr 24, 2019

View reviewed changes

Address comment.

e589a2e

raulchen merged commit f39b674 into ray-project:master Apr 24, 2019

raulchen deleted the command_line_args_parser branch April 24, 2019 06:53

jovany-wang mentioned this pull request May 21, 2019

Fix issue when starting raylet_monitor #4829

Merged

Conversation

jovany-wang commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related Issue

Uh oh!

AmplabJenkins commented Apr 22, 2019

Uh oh!

AmplabJenkins commented Apr 22, 2019

Uh oh!

AmplabJenkins commented Apr 22, 2019

Uh oh!

robertnishihara commented Apr 22, 2019

Uh oh!

jovany-wang commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

AmplabJenkins commented Apr 23, 2019

Uh oh!

robertnishihara commented Apr 23, 2019

Uh oh!

pcmoritz commented Apr 23, 2019

Uh oh!

jovany-wang commented Apr 24, 2019

Uh oh!

raulchen left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

AmplabJenkins commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

jovany-wang commented Apr 20, 2019 •

edited

Loading

jovany-wang commented Apr 23, 2019 •

edited

Loading